Skip to content

Fix cache_key collapse of non-pydantic private/__main__ objects - #259

Merged
timkpaine merged 1 commit into
mainfrom
fix/tokenize-private-module-collapse
Aug 24, 2026
Merged

Fix cache_key collapse of non-pydantic private/__main__ objects#259
timkpaine merged 1 commit into
mainfrom
fix/tokenize-private-module-collapse

Conversation

@ptomecek

@ptomecek ptomecek commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Two different objects could produce the same cache_key, so the cache could return a result computed for a different input. This fixes that.

Cause

The generic fallback in normalize_token (ccflow/utils/tokenize.py) tokenized any object whose type module started with _ or contained ._ by module and qualname alone, discarding the instance state. This was meant to give a stable token to interpreter internals encountered during behavior hashing.

The condition is too broad. "__main__".startswith("_") is True, and private submodules such as pkg._internal contain ._. As a result, any non-pydantic object defined in a notebook, REPL, or private module was reduced to its module and qualname, and distinct values produced identical tokens:

class Add:
    def __init__(self, n): self.n = n
    def __call__(self, x): return x + self.n
Add.__module__ = "pkg._private"

compute_data_token(Add(5)) == compute_data_token(Add(7))  # True

pydantic models, functions, methods, and partials have dedicated handlers and were unaffected; only the fallback path was involved.

Change

Decide based on whether the object can be serialized rather than on its module name. If cloudpickle can serialize the object, its state is folded into the token, and the two Add instances above tokenize distinctly.

The module name is consulted only when serialization fails, to distinguish two cases:

  • Behavior-irrelevant interpreter internals, such as _abc._abc_data (surfaced in ABC class closures on Python 3.14 and not picklable), receive a stable name-only token so behavior hashing does not fail.
  • Any object in __main__, or any other object that cannot be serialized, raises TypeError rather than returning a colliding key.

A short allowlist keeps pydantic's compiled validators name-only. They are picklable but carry volatile runtime state that should not enter the token.

Because the decision is based on serializability rather than a fixed list of internal modules, it holds across Python 3.11 through 3.14 without further maintenance.

Rationale

I instrumented the original branch and ran the full suite to see what reached it. The only objects were the library's own internal helpers, one of which was a stateful frozen dataclass being name-collapsed in the same way (a latent instance of this bug, now also fixed). The _abc and pydantic-internal cases are specific to Python 3.14. This is why an allowlist is the wrong approach: it would require ongoing maintenance and would not have covered the library's own modules.

Tests and validation

Added TestPrivateModuleNoStateCollapse, covering:

  • picklable instances in _secret, pkg._internal, and __main__ tokenize distinctly and deterministically;
  • a stateful frozen dataclass in a private module tokenizes distinctly;
  • an unpicklable user object in __main__ raises TypeError;
  • an unpicklable interpreter-internal object receives a stable name-only token;
  • a picklable pydantic-core-style internal is name-only.

The full suite passes on Python 3.11 (1336 passed, 2 skipped), the core normalize_token logic was re-checked on Python 3.12, and ruff passes.

@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.53%. Comparing base (6a788a6) to head (4f36d52).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
ccflow/tests/utils/test_tokenize.py 98.57% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
+ Coverage   93.48%   93.53%   +0.05%     
==========================================
  Files         176      176              
  Lines       20327    20460     +133     
  Branches     1350     1352       +2     
==========================================
+ Hits        19002    19137     +135     
  Misses       1052     1052              
+ Partials      273      271       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Test Results

    1 files  ± 0      1 suites  ±0   3m 16s ⏱️ +37s
1 347 tests +16  1 345 ✅ +16  2 💤 ±0  0 ❌ ±0 
1 353 runs  +16  1 351 ✅ +16  2 💤 ±0  0 ❌ ±0 

Results for commit 4f36d52. ± Comparison against base commit d8f632d.

♻️ This comment has been updated with latest results.

@ptomecek
ptomecek force-pushed the fix/tokenize-private-module-collapse branch from 026456a to d0d241b Compare August 24, 2026 12:01
@ptomecek
ptomecek marked this pull request as ready for review August 24, 2026 12:06
Comment thread ccflow/utils/tokenize.py Outdated
Comment on lines +113 to +114
if (obj_module.startswith("_") or "._" in obj_module) and obj_module != "__main__":
return ("__internal__", obj_module, type(obj).__qualname__)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This treats __main__ as user code and any other underscore as non-user code, but its reasonable we might have something like company._models, etc. We could consider an explicit list of known used interpreter internals, as opposued to the heuristic of _ prefix.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — replaced the _-prefix heuristic with two explicit curated allowlists, and dug into what actually belongs on them.

I probed the common unpicklable objects. The old failure-path heuristic was already inconsistent: _thread.lock and _abc._abc_data got name-collapsed, but threading.Event, socket.socket, sqlite3.Connection, generators, and weakrefs all fail loud today — purely because their module names don't start with _. So real resources mostly already raise; only two things slipped into name-only.

New behavior:

  • Private framework internals (matched by top-level package pydantic/pydantic_core + any _-prefixed path component) stay name-only. This is picklable-but-volatile compiled state — validators/serializers. Note this version of pydantic puts the validator in pydantic.plugin._schema_validator, which my first narrow list missed; the package+private-component match now covers it, pydantic._internal.*, and pydantic_core._pydantic_core while leaving public modules like pydantic.fields alone.
  • Unpicklable interpreter internals _abc (the 3.14 _abc_data case) and _thread (lock/RLock — primitives with no semantic identity; collapsing them is correct and avoids a regression for any hashed closure that captures a lock) degrade to a stable name-only token.
  • Everything else that can't serialize — DB connections, sockets, threads, and crucially a user class in company._models — now raises TypeError instead of silently colliding.

Added tests covering each bucket, including the real __pydantic_validator__/__pydantic_serializer__ objects and a company._models case. Full suite green on 3.11.

normalize_token's fallback name-only-tokenized any object whose type
module started with "_" or contained "._", silently dropping instance
state. Distinct values (e.g. callables authored in __main__ or a private
module) therefore collapsed onto one cache key, causing false cache hits
and stale results toward the dangerous direction.

Make serializability the primary signal instead of the module name: any
object cloudpickle can serialize now folds its state into the token, so
genuinely-different values stay distinct. The module name is consulted
only via two small curated allowlists:

- Private submodules of the pydantic framework (e.g. pydantic._internal,
  pydantic_core._pydantic_core, pydantic.plugin._schema_validator) expose
  compiled objects that are picklable but carry volatile runtime state;
  they are keyed by module + qualname only, matching prior behavior.
- Unpicklable interpreter internals (_abc._abc_data on Python 3.14, and
  _thread lock/RLock primitives) degrade to a stable name-only token so
  behavior hashing does not crash.

Every other object that fails to serialize -- including anything in
__main__ or a private user package such as company._models -- now raises
loudly rather than silently sharing a key.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Pascal Tomecek <pascal.tomecek@cubistsystematic.com>
@ptomecek
ptomecek force-pushed the fix/tokenize-private-module-collapse branch from d0d241b to 4f36d52 Compare August 24, 2026 15:14
@timkpaine
timkpaine merged commit a143162 into main Aug 24, 2026
20 checks passed
@timkpaine
timkpaine deleted the fix/tokenize-private-module-collapse branch August 24, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants